LONDON | 25-ITP-25 | TEWODROS BEKERE | SPRINT-3 | ToDo-List #735
Conversation
…lete and delete all completed tasks from the todo list and add CSS for a better visualization.
iteddy16
added
📅 Sprint 3
jenny-alexander
added
Review in progress
| <body> | ||
| <div class=" container"> | ||
| <h1> Todo List</h1> | ||
| <form class=""add-todo-form> |
There was a problem hiding this comment.
At line 40, there is a small typo. Can you find it and fix?
| @@ -1,4 +1,4 @@ | |||
| <!DOCTYPE html> | |||
| <!--<!DOCTYPE html> | |||
There was a problem hiding this comment.
It's best to completely remove the commented-out code from the file.
There was a problem hiding this comment.
-
I see a few typos where an extra space has been added before the title of an element or a CSS class name. Can you review your file and fix these typos?
-
💡 Are you using VSCode? If yes, you can right-click within the code, and choose 'Format Document'. This will align your code nicely and make it easier to read!
There was a problem hiding this comment.
While running the Lighthouse accessibility report, a few issues came up that should be fixed:
- The background and foreground colours chosen on the button don't have a high enough contrast ratio, making it hard to read for some people.
- The
<meta name="viewport">tag is missing.
| @@ -24,4 +25,28 @@ | |||
| </form> | |||
| <script src="script.js"></script> | |||
| </body> | |||
| </html>--> | |||
There was a problem hiding this comment.
The --> appears on the UI of your application. Can you adjust your html file so this isn't visible?
| @@ -1,8 +1,41 @@ | |||
| function populateTodoList(todos) { | |||
| let list = document.getElementById("todo-list"); | |||
| // Write your code to create todo list elements with completed and delete buttons here, all todos should display inside the "todo-list" element. | |||
| list.innerHTML = ""; // clears the available list | |||
| if (todos.length === 0) { | |||
There was a problem hiding this comment.
Nice example of an early return at lines 5-8. 👍
There was a problem hiding this comment.
Your JS code is written well and I'm able to interact with your to-do list with ease. I have a items for your to review and implement in order to make your code more robust.
- Replace all
letwithconst(as long as the variable is not being reassigned any values). - Format the javascript code to be easier to read (right-click -> Format Document).
| background-color: #da190b; | ||
| } | ||
|
|
||
| .empty-state { |
There was a problem hiding this comment.
Can you verify if .empty-state is needed?
There was a problem hiding this comment.
This is only a suggestion. You do not need to implement for this PR.
💡Since you are using a lot of different blues and greens within your CSS file, it might be easier to maintain them by defining them at the top of the file. For example:
:root {
--primary-blue: #3936f4;
--primary-green: #085a13;
--accent-red: #dd1024;
}
Then use var(--primary-blue) etc. within your CSS selector definitions.
| } | ||
|
|
||
| // Try to find delete all completed button | ||
| let deleteAllBtn = |
There was a problem hiding this comment.
If there isn't an item in the list, can you think of a way to disable the 'Delete All Complete' button. Ideally, the user shouldn't be able to click on the button if there aren't any items to delete!
jenny-alexander
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.